testers.hasPkgConfigModules: fix bug in `versionCheck` handling #311069

nbraud · 2024-05-12T13:52:53Z

Description of changes

add testcase for the bug reported in testers.hasPkgConfigModules: add optional version check #307770 (comment)
fix bug

Alternative to #310535

Things done

Built on platform(s)
- x86_64-linux
- aarch64-linux
- x86_64-darwin
- aarch64-darwin
Tested, as applicable:
- or, for functions and "core" functionality, tests in lib/tests or pkgs/test
Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

Shell script is currently buggy and effectively ignores the value, always enforcing version match: NixOS#307770 (comment)

Broke tests, as the version check was effectively always enabled: NixOS#307770 (comment)

nbraud · 2024-05-12T13:56:23Z

@roberth Sorry, it was easier to put together an alternative PR, than figure out how to make it through Github's “suggestions” features.

In short, the bug was entirely caused by the broken conditional @OPNA2608 pointed out, and I don't think we need to do anything more complicated around it.

Apologies for the breakage, I forgot to include in the o.g. PR a test for the case where versionCheck is unspecified

nbraud · 2024-05-12T14:02:50Z

Result of nixpkgs-review pr 311069 run on x86_64-linux 1
PS: right, nixpkgs-review does not run tests 😓

nbraud · 2024-05-12T14:08:32Z

I manually checked that python3.tests.pkg-config builds fine now:

❯ nix-build -A python3.tests.pkg-config
[...]
checking pkg-config module python3 in /nix/store/lpi16513bai8kg2bd841745vzk72475x-python3-3.11.9
❌ pkg-config module python3 exists and has version 3.11 when 3.11.9 was expected
[...]
/nix/store/qwvjxvsxf2ylbwbd4izzfnzyjj5gbcaw-check-meta-pkg-config-modules-for-python3-3.11.9

Ideally we'd replace the ❌ with something like ⚠ when versionCheck is false (or unset) ... and I just realised I know how to do it 😅

…tches

roberth · 2024-05-12T18:41:39Z

Good idea to just open a PR. I like your idea, but it needs a bit of work.

I didn't bother with a warning, but if we're going to warn, we should be careful not to cause warning fatigue.
Let's consider the possible combinations:

versionCheck == true:
- ok: report ok
- mismatch: report error
versionCheck == false:
- Caller has decided that they don't want it.
- mismatch: Don't cause warning fatigue. Don't report anything.
- ok: It may seem better to report, but probably the caller has a good reason to disable the check; maybe the version is unreliable instead of always incorrect
unset:
- ok: Caller should set it to true. Tell them to enable the check?
- mismatch: Caller should provide the expected version or disable the check, and we can tell in a message.

What do you think?

nbraud · 2024-05-12T18:52:19Z

I didn't bother with a warning, but if we're going to warn, we should be careful not to cause warning fatigue.

Not sure what you mean, I fixed the extent conditional but didn't add a new warning?
Or did you mean in the tester's log output, rather than a nix-level warning

Right now, callers won't normally see the version-mismatch messages because they are only part of the tester's build log, and not displayed unless the test fails. I added 43efaaa to make it clear that those version mismatches are not test failures (unless versionCheck is true)

I had some thoughs how to gently push callers towards using versionCheck, but that seems out-of-scope for this bugfix PR... and for the 24.05 release cycle TBH

roberth · 2024-05-12T19:45:08Z

Or did you mean in the tester's log output

That's what I meant, yeah.

callers won't normally see the version-mismatch messages because they are only part of the tester's build log,

Users of the package won't, but maintainers and contributors to the package will, and they won't easily understand what's going on.

I had some thoughs how to gently push callers towards using versionCheck, but that seems out-of-scope for this bugfix PR... and for the 24.05 release cycle TBH

Interesting. I agree that we should first fix the error behavior, but I don't think we should trade it for a release that causes warning fatigue. Maybe remove the message altogether when the version check is disabled? Then we can merge that and add a helpful message without hurry.

roberth · 2024-05-12T19:51:03Z

pkgs/build-support/testers/hasPkgConfigModules/tester.nix

+          echo "${if versionCheck then "❌" else "⚠"} pkg-config module $moduleName exists and has version $moduleVersion when $version was expected"
          ((versionMismatch+=1))


To make my suggestion more concrete, from #311069 (comment)

This is a little silly and untested, but I guess it gets the intended behavior across, fwiw.

Suggested change

echo "${if versionCheck then "❌" else "⚠"} pkg-config module $moduleName exists and has version $moduleVersion when $version was expected"

((versionMismatch+=1))

${if versionCheck

then ''

echo "❌ pkg-config module $moduleName exists and has version $moduleVersion when $version was expected"

((versionMismatch+=1))

''

# TODO: figure out the UX for enabling the version check; see https://github.com/NixOS/nixpkgs/pull/311069

else ''

echo "✅ pkg-config module $moduleName exists and has version $moduleVersion"

''}

It's tempting to add more info to the message, but that will just serve to distract whoever reads it, if we don't explain it, which is out of scope for this PR.

I'd prefer more-uniform code, rather than more Bash codegen through text templating; would something like this be fine with you?

Suggested change

echo "${if versionCheck then "❌" else "⚠"} pkg-config module $moduleName exists and has version $moduleVersion when $version was expected"

((versionMismatch+=1))

echo "${if versionCheck then "❌" else "ℹ️"} pkg-config module $moduleName exists at version $moduleVersion != $version (drv version)"

((versionMismatch+=1))

The wording is more neutral, so it seems like a normal info message unless versionCheck
(the versionMismatch counter is unused unless versionCheck, but it's also easier to leave it in)

I think it's useful to be notified regardless that some modules don't match the drv version number (for whatever reason)
If not, there's a much simpler change: moving the [[ -z "$versionCheck" ]] condition

nbraud · 2024-05-12T20:47:11Z

callers won't normally see the version-mismatch messages because they are only part of the tester's build log,

Users of the package won't, but maintainers and contributors to the package will, and they won't easily understand what's going on.
[...]
Interesting. I agree that we should first fix the error behavior, but I don't think we should trade it for a release that causes warning fatigue. Maybe remove the message altogether when the version check is disabled? Then we can merge that and add a helpful message without hurry.

I find it odd to talk about “warning fatigue” in that context, since it's usually meant for warnings emitted in nix evaluation.

Regardless, I feel the issue is somewhat overstated, since the message is only ever visible when explicitely looking at the build log of foo.tests.pkg-config. It's not visible when evaluating the drv, or looking at the drv's build log.

In any case, we could easily just not emit the warning at all unless versionCheck if that's a blocker

…n mismatches

roberth · 2024-05-14T20:00:47Z

LGTM if you could rebase away the fixup.

I find it odd to talk about “warning fatigue” in that context, since it's usually meant for warnings emitted in nix evaluation.

It's a concept that's applicable to all warnings. Eval warnings can be more visible, so it's worse there. Still bad in build logs though.

In any case, we could easily just not emit the warning at all unless versionCheck if that's a blocker

Let's just fix it first. We can get into friendly nudging later.

roberth · 2024-05-20T22:13:20Z

Thanks!

nbraud · 2024-05-26T12:59:20Z

Thanks for the merge, @roberth, and sorry for the lack of reply: I was out of the country for the RIPE88 meeting

nbraud added 2 commits May 12, 2024 13:49

tests.testers.hasPkgConfigModules: test the versionCheck parameter

1d4d9fa

Shell script is currently buggy and effectively ignores the value, always enforcing version match: NixOS#307770 (comment)

testers.hasPkgConfigModules: fix versionCheck conditional

4af3b80

Broke tests, as the version check was effectively always enabled: NixOS#307770 (comment)

nbraud requested a review from roberth May 12, 2024 13:53

nbraud mentioned this pull request May 12, 2024

Fix testers.hasPkgConfigModules version check #310535

Closed

13 tasks

testers.hasPkgConfigModules: don't display ❌ on ignored version misma…

43efaaa

…tches

nbraud force-pushed the fix-307770 branch from 2a03cae to 43efaaa Compare May 12, 2024 14:13

ofborg bot added 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 101-500 10.rebuild-linux: 101-500 labels May 12, 2024

nbraud mentioned this pull request May 12, 2024

mir: Fix tests #310091

Merged

13 tasks

roberth reviewed May 12, 2024

View reviewed changes

fixup! testers.hasPkgConfigModules: don't display ❌ on ignored versio…

bf4d46e

…n mismatches

midchildan mentioned this pull request May 15, 2024

ncurses: Build standard terminfo dirs into static ncurses #311411

Merged

13 tasks

roberth merged commit 28df229 into NixOS:master May 20, 2024
20 checks passed

nbraud deleted the fix-307770 branch May 26, 2024 12:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

testers.hasPkgConfigModules: fix bug in `versionCheck` handling #311069

testers.hasPkgConfigModules: fix bug in `versionCheck` handling #311069

nbraud commented May 12, 2024 •

edited

Loading

nbraud commented May 12, 2024

nbraud commented May 12, 2024 •

edited

Loading

nbraud commented May 12, 2024

roberth commented May 12, 2024

nbraud commented May 12, 2024

roberth commented May 12, 2024

roberth May 12, 2024

roberth May 12, 2024

nbraud May 12, 2024 •

edited

Loading

nbraud May 12, 2024

nbraud commented May 12, 2024

roberth commented May 14, 2024

roberth commented May 20, 2024

nbraud commented May 26, 2024

		echo "${if versionCheck then "❌" else "⚠"} pkg-config module $moduleName exists and has version $moduleVersion when $version was expected"
		((versionMismatch+=1))

testers.hasPkgConfigModules: fix bug in versionCheck handling #311069

testers.hasPkgConfigModules: fix bug in versionCheck handling #311069

Conversation

nbraud commented May 12, 2024 • edited Loading

Description of changes

Things done

nbraud commented May 12, 2024

nbraud commented May 12, 2024 • edited Loading

nbraud commented May 12, 2024

roberth commented May 12, 2024

nbraud commented May 12, 2024

roberth commented May 12, 2024

roberth May 12, 2024

Choose a reason for hiding this comment

roberth May 12, 2024

Choose a reason for hiding this comment

nbraud May 12, 2024 • edited Loading

Choose a reason for hiding this comment

nbraud May 12, 2024

Choose a reason for hiding this comment

nbraud commented May 12, 2024

roberth commented May 14, 2024

roberth commented May 20, 2024

nbraud commented May 26, 2024

testers.hasPkgConfigModules: fix bug in `versionCheck` handling #311069

testers.hasPkgConfigModules: fix bug in `versionCheck` handling #311069

nbraud commented May 12, 2024 •

edited

Loading

nbraud commented May 12, 2024 •

edited

Loading

nbraud May 12, 2024 •

edited

Loading